Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the unused StableSet and StableMap types from rustc_data_structures. #99058

Merged
merged 2 commits into from
Jul 21, 2022

Conversation

michaelwoerister
Copy link
Member

The current implementation is not "stable" in the same sense that HashStable and StableHasher are stable, i.e. across compilation sessions. So, in my opinion, it's better to remove those types (which are basically unused anyway) than to give the wrong impression that these are safe for incr. comp.

I plan to provide new "stable" collection types soon that can be used to replace FxHashMap and FxHashSet in query results (see draft). It's unsound that HashMap and HashSet implement HashStable (see #98890 for a recent P-critical bug caused by this) -- so we should make some progress there.

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 8, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 8, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

@rust-highfive
Copy link
Collaborator

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 8, 2022
@rust-log-analyzer

This comment has been minimized.

@michaelwoerister
Copy link
Member Author

See #94188 for some prior work in this direction. I'll probably create an MCP for removing the HashStable implementation from HashMap and HashSet.

@cjgillot
Copy link
Contributor

cjgillot commented Jul 8, 2022

#98890 was not caused by a FxHashMap getting returned by a query. It was caused by a FxHashMap getting iterated upon. I think the proper fix for this class of bugs is the internal lint forbidding iteration over hash-maps.

@michaelwoerister
Copy link
Member Author

In the MCP write-up I'll explain in more detail how I think things fit together. But in short, I think it's preferable to just not implement HashStable for HashMap and HashSet (and BTreeMap and BTreeSet). They don't fulfill the contract that any observable difference results in a different Fingerprint -- so I think it's hard to argue that they should nonetheless implement HashStable.

I think the proper fix for this class of bugs is the internal lint forbidding iteration over hash-maps.

I think the solution is a combination of both. If we disallow types in query results that don't have a proper HashStable impl, then indeterminism coming from iterating over a HashSet will result in spurious cache invalidation instead of potential soundness errors -- so that's an improvement. But we don't want the spurious invalidations either, so the lint will be useful to avoid those too.

@nagisa
Copy link
Member

nagisa commented Jul 16, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jul 16, 2022

📌 Commit 2d2ac32 has been approved by nagisa

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 16, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 18, 2022
…and-map, r=nagisa

Remove the unused StableSet and StableMap types from rustc_data_structures.

The current implementation is not "stable" in the same sense that `HashStable` and `StableHasher` are stable, i.e. across compilation sessions. So, in my opinion, it's better to remove those types (which are basically unused anyway) than to give the wrong impression that these are safe for incr. comp.

I plan to provide new "stable" collection types soon that can be used to replace `FxHashMap` and `FxHashSet` in query results (see [draft](michaelwoerister@69d03ac)). It's unsound that `HashMap` and `HashSet` implement `HashStable` (see rust-lang#98890 for a recent P-critical bug caused by this) -- so we should make some progress there.
@Dylan-DPC
Copy link
Member

failed in rollup ci

@Dylan-DPC
Copy link
Member

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 18, 2022
@bors
Copy link
Contributor

bors commented Jul 19, 2022

☔ The latest upstream changes (presumably #99451) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@michaelwoerister
Copy link
Member Author

Rebased.

Don't include this in rollups since it can easily break if another PR adds a use rustc_data_structures::stable_map::FxHashMap before this is being tested.

@bors r=nagisa rollup=never

@bors
Copy link
Contributor

bors commented Jul 20, 2022

📌 Commit 88f6c6d has been approved by nagisa

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 20, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 20, 2022
@bors
Copy link
Contributor

bors commented Jul 20, 2022

⌛ Testing commit 88f6c6d with merge be9cfb3...

@bors
Copy link
Contributor

bors commented Jul 21, 2022

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing be9cfb3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 21, 2022
@bors bors merged commit be9cfb3 into rust-lang:master Jul 21, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 21, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (be9cfb3): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
2.2% 2.2% 1
Improvements 🎉
(primary)
-2.7% -2.7% 1
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) -2.7% -2.7% 1

Cycles

This benchmark run did not return any relevant results for this metric.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change

  2. number of relevant changes

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling rustc_expand v0.0.0 (/checkout/compiler/rustc_expand)
error[E0282]: type annotations needed
   --> compiler/rustc_middle/src/mir/terminator.rs:294:42
    |
294 |             1 => write!(fmt, " -> {:?}", self.successors().next().unwrap()),
    |                  |                       |
    |                  |                       |
    |                  |                       cannot infer type of the type parameter `T` declared on the associated function `new_debug`
    |
   ::: /checkout/library/core/src/macros/mod.rs:498:1
    |
498 | macro_rules! write {
498 | macro_rules! write {
    | ------------------ in this expansion of `write!` (#1)
499 |     ($dst:expr, $($arg:tt)*) => {{
500 |         let result = $dst.write_fmt($crate::format_args!($($arg)*));
...
879 |     macro_rules! format_args {
    |     ------------------------ in this expansion of `$crate::format_args!` (#2)
    |
    |
help: consider specifying the generic argument
    |
294 |             1 => write!(fmt, " -> {:?}", self.successors().next().unwrap()::<T>),

   Compiling rustc_builtin_macros v0.0.0 (/checkout/compiler/rustc_builtin_macros)
error[E0282]: type annotations needed
    --> compiler/rustc_middle/src/ty/mod.rs:1786:21
    --> compiler/rustc_middle/src/ty/mod.rs:1786:21
     |
1786 |         for attr in tcx.get_attrs(did, sym::repr) {

error[E0282]: type annotations needed
    --> compiler/rustc_middle/src/ty/mod.rs:2171:35
     |
     |
2171 |         self.get_attrs(did, attr).next()
     |
help: try using a fully qualified path to specify the expected types
     |
     |
2171 |         <Self as Iterator>::next(&mut self.get_attrs(did, attr))

error[E0282]: type annotations needed
    --> compiler/rustc_middle/src/ty/mod.rs:2179:39
     |
     |
2179 |             self.get_attrs(did, attr).next().is_some()
     |
help: try using a fully qualified path to specify the expected types
     |
     |
2179 |             <Self as Iterator>::next(&mut self.get_attrs(did, attr)).is_some()

For more information about this error, try `rustc --explain E0282`.
error: could not compile `rustc_middle` due to 4 previous errors
warning: build failed, waiting for other jobs to finish...

celinval added a commit to celinval/kani-dev that referenced this pull request Aug 8, 2022
celinval added a commit to celinval/kani-dev that referenced this pull request Aug 15, 2022
celinval added a commit to model-checking/kani that referenced this pull request Aug 17, 2022
* Fix compilation errors

Regression is still failing. Related changes:

- rust-lang/rust#99420
- rust-lang/rust#99495
- rust-lang/rust#99844
- rust-lang/rust#99058

* Change test to expect compilation failure

The compiler has reverted their fix to Opaque types due to performance
degradation.

* Fix VTable handling now that it has an Opaque type

 - Add an implementation for vtable_size and vtable_align intrinsics.
 - Change how we handled Foreign types. Even though they are unsized, a
   pointer to foreign types is a thin pointer.

Co-authored-by: Daniel Schwartz-Narbonne <danielsn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants